Support Slack v2 OAuth in components #18744
Conversation
- List users by real name in dropdowns - List users by real name in action summary - List files using bot token if available - Get files using bot token if available - Show configuration error if bot is not a member of the channel - Add `addToChannel` prop to Send Message actions - Check user's access to channel in message actions - Use RTS API to search messages in Find Message action - List only channels, not DMs, in 'file' actions - List only channels, not DMs, in 'message' triggers - Handle permission error fetching message in New Reaction trigger Context: In Slack's v2 OAuth flow, user access tokens and bot access tokens are issued with separate scopes. User tokens are used to perform actions on behalf of a user, while bot tokens are used for actions performed by the app itself. To send a message to a private channel or DM, the bot must be a member of that channel. Due to permission restrictions, some endpoints require bot tokens for access, such as viewing messages in DMs and listing files. As a result, access to messages and files in DMs is no longer supported.
This allows us to support both existing and new Slack connections created with Slack's v2 OAuth flow, with or without the deprecated perspectival chat:write:user and chat:write:bot scopes: | Slack auth | Token Types | Scopes | as_user Values | |------------|-------------|-----------------------|-----------------| | v1 | user | chat:write:bot, :user | true, false | | v2 | user, bot | chat:write:bot, :user | true, false | | v2 | user, bot | chat:write | true | In v2, when the chat:write scope is requested, it replaces chat:write:bot and chat:write:user. Without the chat:write:bot scope, as_user cannot be false; user tokens always post as the user, and bot tokens always post as the bot. In v2, with or without the chat:write:bot scope, we can use the bot token if as_user is false since it will have permission to post as itself. And we MAY pass through as_user if it's true since Slack allows it even when it's superfluous. However, we MUST pass through as_user if it's true AND the user token still has the chat:write:bot scope since otherwise the message will post as the bot user instead of the user. See: https://docs.slack.dev/reference/methods/chat.postMessage/#legacy_as_user
so it is shown in the component form by default so users aren't suprised by the new invite behavior
it's no longer supported for new auths since they will not have the im:history scope
as requested by Slack
Fallback to search.messages API method if missing scopese
Future-proofs Slack components to support proxying requests
add/update logged messages when unable to add app to channel
The following components have new minor versions: - delete-message - find-message - get-file - list-files - reply-to-a-message - send-block-kit-message - send-large-message - send-message - send-message-advanced - send-message-to-channel - send-message-to-user-or-group - update-message - new-keyword-mention - new-message-in-channels - new-user-mention All other Slack components have new patch versions.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughSlack component receives extensive updates: token handling, bot-token flows, name resolution, assistantSearch, maybeAddAppToChannels; many Slack actions/sources have version bumps, some gain addToChannel and conversation-type constraints; find-message rewritten; a signature action and direct-message source (and test event) were removed. Several non-Slack files only had formatting fixes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Action as Slack Action
participant App as Slack App
participant Slack as Slack API
User->>Action: invoke (props include conversation, addToChannel?)
alt addToChannel = true
Action->>App: maybeAddAppToChannels([conversation])
App->>App: getBotToken() / getToken({asBot:true})
App->>Slack: invite/join or relevant API using bot token
Slack-->>App: ok / error
end
Action->>App: sdk/makeRequest(..., asBot/as_user?)
App->>App: select token based on opts
App->>Slack: API call (chat.postMessage, files.list, files.info, etc.)
Slack-->>App: response
App-->>Action: result (uses getChannelDisplayName for summaries)
Action-->>User: final output
sequenceDiagram
autonumber
actor User
participant Find as Find-Message Action
participant App as Slack App
participant Slack as Slack API
User->>Find: Run({query, maxResults})
Find->>App: assistantSearch(baseParams)
App->>Slack: assistant.search
alt assistant.search success
Slack-->>App: matches
App-->>Find: normalized results
else assistant.search error
Note right of Find: if shouldFallbackToSearchMessages(error)
Find->>App: searchMessages with pagination
App->>Slack: search.messages (paginated)
Slack-->>App: results until exhausted or maxResults
App-->>Find: accumulated messages
end
Find-->>User: messages (up to maxResults)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/slack/actions/send-large-message/send-large-message.mjs (1)
49-64: Fix metadata_event_payload handling to support object inputs.When
metadata_event_payloadis provided as an object (not a string),metadataEventPayloadremains undefined, resulting inevent_payload: undefinedin the metadata object. The code only assignsmetadataEventPayloadwhen parsing a string.Apply this diff to handle both string and object inputs:
- let metadataEventPayload; + let metadataEventPayload = this.metadata_event_payload; if (this.metadata_event_type) { if (typeof this.metadata_event_payload === "string") { try { metadataEventPayload = JSON.parse(this.metadata_event_payload); } catch (error) { throw new Error(`Invalid JSON in metadata_event_payload: ${error.message}`); } } this.metadata = { event_type: this.metadata_event_type, event_payload: metadataEventPayload, }; }components/slack/actions/common/send-message.mjs (1)
213-229: Fix metadata_event_payload handling to support object inputs.When
metadata_event_payloadis provided as an object (not a string),metadataEventPayloadremains undefined, resulting inevent_payload: undefinedin the metadata object. The code only assignsmetadataEventPayloadwhen parsing a string.Apply this diff to handle both string and object inputs:
- let metadataEventPayload; + let metadataEventPayload = this.metadata_event_payload; if (this.metadata_event_type) { if (typeof this.metadata_event_payload === "string") { try { metadataEventPayload = JSON.parse(this.metadata_event_payload); } catch (error) { throw new Error(`Invalid JSON in metadata_event_payload: ${error.message}`); } } this.metadata = { event_type: this.metadata_event_type, event_payload: metadataEventPayload, }; }
🧹 Nitpick comments (1)
components/slack/actions/update-group-members/update-group-members.mjs (1)
54-55: Good defensive fix!Adding default empty arrays prevents
TypeErrorwhen the optional arrays are undefined, since line 62 calls.includes()and line 63 spreads the array.Optionally, if a user appears in both the existing group members and
usersToAdd, they may be duplicated in the final array (line 63). Consider verifying whether the Slack API deduplicates on their end, or add explicit deduplication if needed:users = users.filter((user) => !usersToRemove.includes(user)); - users.push(...usersToAdd); + const allUsers = [...new Set([...users, ...usersToAdd])]; const response = await this.slack.updateGroupMembers({ usergroup: userGroup, - users, + users: allUsers, team_id: team, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
components/azure_cosmos_db/azure_cosmos_db.app.mjs(1 hunks)components/binalyze_air/binalyze_air.app.mjs(1 hunks)components/brainbase_labs/brainbase_labs.app.mjs(1 hunks)components/google_perspective/google_perspective.app.mjs(1 hunks)components/n1n/n1n.app.mjs(1 hunks)components/slack/actions/add-emoji-reaction/add-emoji-reaction.mjs(1 hunks)components/slack/actions/approve-workflow/approve-workflow.mjs(1 hunks)components/slack/actions/archive-channel/archive-channel.mjs(1 hunks)components/slack/actions/common/send-message.mjs(4 hunks)components/slack/actions/create-channel/create-channel.mjs(1 hunks)components/slack/actions/create-reminder/create-reminder.mjs(1 hunks)components/slack/actions/delete-file/delete-file.mjs(1 hunks)components/slack/actions/delete-message/delete-message.mjs(1 hunks)components/slack/actions/find-message/find-message.mjs(3 hunks)components/slack/actions/find-user-by-email/find-user-by-email.mjs(1 hunks)components/slack/actions/get-file/get-file.mjs(3 hunks)components/slack/actions/invite-user-to-channel/invite-user-to-channel.mjs(1 hunks)components/slack/actions/kick-user/kick-user.mjs(1 hunks)components/slack/actions/list-channels/list-channels.mjs(1 hunks)components/slack/actions/list-files/list-files.mjs(3 hunks)components/slack/actions/list-group-members/list-group-members.mjs(1 hunks)components/slack/actions/list-members-in-channel/list-members-in-channel.mjs(1 hunks)components/slack/actions/list-replies/list-replies.mjs(2 hunks)components/slack/actions/list-users/list-users.mjs(1 hunks)components/slack/actions/reply-to-a-message/reply-to-a-message.mjs(1 hunks)components/slack/actions/send-block-kit-message/send-block-kit-message.mjs(1 hunks)components/slack/actions/send-large-message/send-large-message.mjs(3 hunks)components/slack/actions/send-message-advanced/send-message-advanced.mjs(1 hunks)components/slack/actions/send-message-to-channel/send-message-to-channel.mjs(1 hunks)components/slack/actions/send-message-to-user-or-group/send-message-to-user-or-group.mjs(2 hunks)components/slack/actions/send-message/send-message.mjs(1 hunks)components/slack/actions/set-channel-description/set-channel-description.mjs(1 hunks)components/slack/actions/set-channel-topic/set-channel-topic.mjs(1 hunks)components/slack/actions/set-status/set-status.mjs(1 hunks)components/slack/actions/update-group-members/update-group-members.mjs(2 hunks)components/slack/actions/update-message/update-message.mjs(1 hunks)components/slack/actions/update-profile/update-profile.mjs(1 hunks)components/slack/actions/upload-file/upload-file.mjs(1 hunks)components/slack/actions/verify-slack-signature/verify-slack-signature.mjs(0 hunks)components/slack/package.json(1 hunks)components/slack/slack.app.mjs(9 hunks)components/slack/sources/new-channel-created/new-channel-created.mjs(1 hunks)components/slack/sources/new-direct-message/new-direct-message.mjs(0 hunks)components/slack/sources/new-direct-message/test-event.mjs(0 hunks)components/slack/sources/new-interaction-event-received/new-interaction-event-received.mjs(1 hunks)components/slack/sources/new-keyword-mention/new-keyword-mention.mjs(2 hunks)components/slack/sources/new-message-in-channels/new-message-in-channels.mjs(2 hunks)components/slack/sources/new-reaction-added/new-reaction-added.mjs(2 hunks)components/slack/sources/new-saved-message/new-saved-message.mjs(1 hunks)components/slack/sources/new-user-added/new-user-added.mjs(1 hunks)components/slack/sources/new-user-mention/new-user-mention.mjs(2 hunks)
💤 Files with no reviewable changes (3)
- components/slack/sources/new-direct-message/new-direct-message.mjs
- components/slack/actions/verify-slack-signature/verify-slack-signature.mjs
- components/slack/sources/new-direct-message/test-event.mjs
🧰 Additional context used
🧬 Code graph analysis (3)
components/slack/actions/common/send-message.mjs (2)
components/slack/slack.app.mjs (3)
channelId(24-27)channel(505-507)channel(514-516)components/slack/actions/send-large-message/send-large-message.mjs (1)
channelName(91-91)
components/slack/actions/send-large-message/send-large-message.mjs (1)
components/slack/slack.app.mjs (2)
channel(505-507)channel(514-516)
components/slack/slack.app.mjs (2)
components/slack/actions/get-file/get-file.mjs (1)
response(53-55)components/slack/actions/find-message/find-message.mjs (3)
response(115-119)response(134-138)cursor(112-112)
🔇 Additional comments (29)
components/slack/sources/new-channel-created/new-channel-created.mjs (1)
8-8: Version bump looks good.No issues spotted with the update to version 0.0.11.
components/slack/actions/archive-channel/archive-channel.mjs (1)
8-8: Version bump looks good.Version increment to 0.0.25 is consistent and introduces no regressions.
components/slack/actions/set-channel-topic/set-channel-topic.mjs (1)
7-7: Version bump looks good.Matches surrounding Slack action updates; no further changes needed.
components/slack/actions/update-profile/update-profile.mjs (1)
8-8: Version bump looks good.Increment to 0.0.25 keeps this action aligned with the other Slack updates in the PR. No further action needed.
components/slack/actions/approve-workflow/approve-workflow.mjs (1)
8-8: Version bump looks good.Matches the broader Slack release versioning pattern with no functional impact.
components/slack/sources/new-user-added/new-user-added.mjs (1)
8-8: Version bump looks goodThe source metadata increment is consistent and introduces no risk.
components/slack/actions/create-reminder/create-reminder.mjs (1)
7-7: Version bump looks goodThe metadata-only bump to 0.0.26 keeps this action aligned with the broader Slack release. 👍
components/slack/actions/send-block-kit-message/send-block-kit-message.mjs (1)
10-10: Version bump looks good.Increment aligns with the broader Slack v2 rollout; no further action needed.
components/slack/actions/kick-user/kick-user.mjs (1)
8-8: Version bump looks consistent.Matches the broader component release cadence. No further issues spotted.
components/azure_cosmos_db/azure_cosmos_db.app.mjs (1)
11-11: LGTM — object terminator restored.Thanks for reintroducing the closing
};; keeps the module syntax tidy.components/slack/actions/reply-to-a-message/reply-to-a-message.mjs (1)
9-9: Version bump looks goodIncrement to
0.2.0is consistent with the ongoing Slack updates in this PR.components/slack/sources/new-saved-message/new-saved-message.mjs (1)
8-8: Version bump looks good.The patch version increment is appropriate for this maintenance update.
Note: Line 9 mentions this endpoint is deprecated by Slack and may be shut down in the future. Consider tracking this deprecation to ensure timely migration or removal when Slack discontinues support.
components/slack/actions/find-user-by-email/find-user-by-email.mjs (1)
7-7: LGTM! Version bump aligns with OAuth v2 compatibility.The version increment appropriately marks this action as compatible with the underlying Slack OAuth v2 changes, even though no logic changes were required.
components/slack/actions/list-channels/list-channels.mjs (1)
7-7: Verify that no functional changes are needed for v2 OAuth support.The version bump appears intentional for indicating v2 OAuth compatibility. Since this is a read-only action that only lists channels (as indicated by the
readOnlyHint: trueannotation), it likely doesn't require the additional props (likeaddToChannel) or pre-run logic that other Slack actions in this PR received.However, please confirm that the underlying
slack.app.mjscomponent handles all necessary v2 OAuth changes transparently, and that this action will function correctly without any code modifications.components/slack/actions/add-emoji-reaction/add-emoji-reaction.mjs (1)
7-7: LGTM: Version bump for coordinated release.The metadata-only version bump aligns with the coordinated release pattern across Slack actions in this PR.
components/slack/actions/update-group-members/update-group-members.mjs (1)
7-7: LGTM!Version bump is appropriate for this defensive code improvement.
components/slack/actions/list-replies/list-replies.mjs (3)
1-1: LGTM!The constants import is correctly added and used for channel type constraints.
8-8: LGTM!The version bump from 0.0.24 to 0.0.25 is appropriate for this change.
21-28: Confirm DM/group channel support isn't required: list-replies now limits the picker to CHANNEL_TYPE.PUBLIC and CHANNEL_TYPE.PRIVATE—verify no existing workflows depend on CHANNEL_TYPE.IM or CHANNEL_TYPE.MPIM.components/slack/sources/new-reaction-added/new-reaction-added.mjs (2)
8-8: LGTM: Version bump aligns with error handling changes.The minor version increment from
1.1.26to1.2.0appropriately reflects the defensive error handling added toprocessEvent.
100-107: Approve with observability enhancements
- Replace console.log with a structured logger or emit a metric on getMessage failure.
- Downstream Slack handlers do not reference event.message, so undefined messages won’t break processing.
- If message retrieval is critical, attach an error field to the returned event for better troubleshooting.
components/slack/actions/send-large-message/send-large-message.mjs (2)
38-42: LGTM! Pre-send channel invitation logic is correct.The conditional check for
addToChanneland subsequent call tomaybeAddAppToChannelsensures the app is properly invited to the channel before sending the message. TheaddToChannelprop is correctly inherited from the common send-message module.
91-91: LGTM! Channel name resolution refactored to use helper.Using
getChannelDisplayNamecentralizes the channel display logic and improves maintainability.components/slack/sources/new-user-mention/new-user-mention.mjs (1)
20-25: LGTM! Channel type constraints correctly applied.The dynamic type constraints limit conversation selection to public and private channels using shared constants, aligning with the pattern adopted across other Slack sources and actions in this PR.
components/slack/actions/common/send-message.mjs (2)
12-17: LGTM! addToChannel prop and pre-send logic implemented correctly.The new
addToChannelprop allows users to opt into automatic channel invitation, and the pre-send logic efficiently cacheschannelIdand invokesmaybeAddAppToChannelswhen needed.Also applies to: 190-196
258-258: LGTM! Channel name resolution refactored to use helper.Using
getChannelDisplayNamecentralizes the channel display logic and improves maintainability.components/slack/sources/new-keyword-mention/new-keyword-mention.mjs (1)
20-25: LGTM! Channel type constraints correctly applied.The dynamic type constraints limit conversation selection to public and private channels using shared constants, consistent with the pattern in new-user-mention.mjs and other Slack components.
components/slack/actions/get-file/get-file.mjs (1)
21-29: LGTM! All changes follow the established pattern.The channel type constraints,
addToChannelprop, and pre-run invitation logic are correctly implemented and consistent with the pattern adopted across other Slack actions in this PR.Also applies to: 30-35, 47-51
components/slack/actions/list-files/list-files.mjs (1)
21-29: LGTM! All changes follow the established pattern.The channel type constraints,
addToChannelprop, and pre-run invitation logic are correctly implemented and consistent with the pattern adopted across other Slack actions in this PR.Also applies to: 30-35, 64-68
| if ([ | ||
| "not_in_channel", | ||
| "channel_not_found", | ||
| ].some((errorType) => error.includes(errorType)) && asBot) { | ||
| const followUp = method.startsWith("chat.") | ||
| ? "Ensure the bot is a member of the channel, or set the **Send as User** option to true to act on behalf of the authenticated user." | ||
| : "Ensure the bot is a member of the channel."; | ||
| throw new ConfigurationError(`${error}\n${followUp}`); | ||
| } |
There was a problem hiding this comment.
Fix error handling before it throws a TypeError
error is typically an Error object (e.g., WebAPIPlatformError), so calling error.includes(...) throws TypeError: error.includes is not a function, hiding the original Slack error instead of surfacing it. Coerce the error to a string (or read error.data?.error) before running .includes, and use that value in the ConfigurationError message.
- if ([
- "not_in_channel",
- "channel_not_found",
- ].some((errorType) => error.includes(errorType)) && asBot) {
- const followUp = method.startsWith("chat.")
- ? "Ensure the bot is a member of the channel, or set the **Send as User** option to true to act on behalf of the authenticated user."
- : "Ensure the bot is a member of the channel.";
- throw new ConfigurationError(`${error}\n${followUp}`);
- }
- throw error;
+ const errorMessage = typeof error === "string"
+ ? error
+ : error?.data?.error || error.message || `${error}`;
+ if ([
+ "not_in_channel",
+ "channel_not_found",
+ ].some((errorType) => errorMessage.includes(errorType)) && asBot) {
+ const followUp = method.startsWith("chat.")
+ ? "Ensure the bot is a member of the channel, or set the **Send as User** option to true to act on behalf of the authenticated user."
+ : "Ensure the bot is a member of the channel.";
+ throw new ConfigurationError(`${errorMessage}\n${followUp}`);
+ }
+ throw error;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ([ | |
| "not_in_channel", | |
| "channel_not_found", | |
| ].some((errorType) => error.includes(errorType)) && asBot) { | |
| const followUp = method.startsWith("chat.") | |
| ? "Ensure the bot is a member of the channel, or set the **Send as User** option to true to act on behalf of the authenticated user." | |
| : "Ensure the bot is a member of the channel."; | |
| throw new ConfigurationError(`${error}\n${followUp}`); | |
| } | |
| const errorMessage = typeof error === "string" | |
| ? error | |
| : error?.data?.error || error.message || `${error}`; | |
| if ([ | |
| "not_in_channel", | |
| "channel_not_found", | |
| ].some((errorType) => errorMessage.includes(errorType)) && asBot) { | |
| const followUp = method.startsWith("chat.") | |
| ? "Ensure the bot is a member of the channel, or set the **Send as User** option to true to act on behalf of the authenticated user." | |
| : "Ensure the bot is a member of the channel."; | |
| throw new ConfigurationError(`${errorMessage}\n${followUp}`); | |
| } | |
| throw error; |
🤖 Prompt for AI Agents
In components/slack/slack.app.mjs around lines 561 to 569, the code assumes
`error` is a string and calls `error.includes(...)`, which throws a TypeError
when `error` is an Error object; change the logic to extract a safe string first
(e.g., const errMsg = typeof error === "string" ? error : error?.data?.error ??
String(error)), use `errMsg.includes(...)` for the conditional, and use `errMsg`
when constructing the ConfigurationError so the original Slack error is
preserved and no TypeError is thrown.
guard against unexpected types
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/slack/actions/find-message/find-message.mjs (1)
42-42: Fix stray backtick in description.Clean up the string.
- description: "Sort ascending (asc) or descending (desc)`", + description: "Sort ascending (asc) or descending (desc)",
♻️ Duplicate comments (1)
components/slack/slack.app.mjs (1)
561-569: Implement the error message extraction suggested in the previous review.While
${error}prevents the TypeError, the previous review correctly noted that extractingerror.data?.errorwould preserve the original Slack error message more accurately. The current string coercion might lose structured error details.Apply the diff suggested in the previous review to properly extract and preserve error details.
🧹 Nitpick comments (7)
components/slack/slack.app.mjs (1)
502-527: Consider logging caught errors for debugging.While the fallback strings provide good UX when name resolution fails, silently catching errors at lines 509 and 522 could hide legitimate issues (e.g., auth failures, rate limits). Consider adding
console.error(err)or similar logging in the catch blocks to aid troubleshooting.components/slack/actions/find-message/find-message.mjs (6)
80-90: Normalize merge precedence to avoid accidental overrides.Currently, ...baseMessage and ...rest can override canonical fields (user, ts, text, type, channel). If the canonical fields should win, flip the spread order.
- const normalized = { - type: "message", - user: authorUserId, - team: teamId, - ts: messageTs, - text: content, - permalink, - channel: channelInfo, - ...baseMessage, - ...rest, - }; + const normalized = { + ...baseMessage, + ...rest, + type: "message", + user: authorUserId, + team: teamId, + ts: messageTs, + text: content, + permalink, + channel: channelInfo, + };If overriding by baseMessage/rest is intentional, consider adding a brief comment to document the precedence.
115-119: Align with existing Slack client usage: pass throwRateLimitError.Other Slack app calls set throwRateLimitError: true. Consider adding it here for consistency and clearer retry behavior. Based on relevant code snippets.
- const response = await this.slack.assistantSearch({ + const response = await this.slack.assistantSearch({ ...baseParams, channel_types: "public_channel,private_channel", cursor, + throwRateLimitError: true, });
117-118: Channel types exclude DMs — intentional?assistantSearch is limited to public/private channels. If you also want to match IM/MPIM messages, add im,mpim or make it configurable.
- channel_types: "public_channel,private_channel", + channel_types: "public_channel,private_channel,im,mpim",
134-138: Also pass throwRateLimitError to searchMessages.Match the pattern used across the Slack app for consistent error semantics. Based on relevant code snippets.
- const response = await this.slack.searchMessages({ + const response = await this.slack.searchMessages({ ...baseParams, count, page, + throwRateLimitError: true, });
190-194: Avoid sending undefined params to the API.Only include sort/sort_dir when set to reduce risk of invalid query params.
- const baseParams = { - query: this.query, - sort: this.sort, - sort_dir: this.sortDirection, - }; + const baseParams = { + query: this.query, + ...(this.sort && { sort: this.sort }), + ...(this.sortDirection && { sort_dir: this.sortDirection }), + };
197-205: Optional: Fallback if assistantSearch is unavailable.If older app versions or tokens don’t expose assistantSearch, pre-check and use searchMessages directly.
- try { - matches = await this.searchWithAssistant(baseParams, maxResults); - } catch (error) { + try { + if (typeof this.slack.assistantSearch !== "function") { + matches = await this.searchWithSearchMessages(baseParams, maxResults); + } else { + matches = await this.searchWithAssistant(baseParams, maxResults); + } + } catch (error) { if (this.shouldFallbackToSearchMessages(error)) { matches = await this.searchWithSearchMessages(baseParams, maxResults); } else { throw error; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/slack/actions/find-message/find-message.mjs(3 hunks)components/slack/slack.app.mjs(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/slack/actions/find-message/find-message.mjs (1)
components/slack/slack.app.mjs (8)
channel(505-507)channel(514-516)cursor(187-196)cursor(240-247)cursor(629-629)cursor(653-653)response(557-557)page(293-298)
components/slack/slack.app.mjs (2)
components/slack/actions/find-message/find-message.mjs (3)
response(115-119)response(134-138)cursor(112-112)components/slack/actions/get-file/get-file.mjs (1)
response(53-55)
🔇 Additional comments (9)
components/slack/slack.app.mjs (7)
5-5: LGTM: ConfigurationError import supports better error handling.The import enables throwing user-friendly configuration errors when bot permissions are insufficient, improving the developer experience.
31-32: LGTM: Name resolution improvements are well-implemented.The changes correctly:
- Filter out falsy user IDs before lookup
- Optimize by resolving IDs and usernames in parallel
- Extract usernames from MPIM purpose values using regex
The inline comment at lines 140-142 clearly explains the optimization rationale.
Also applies to: 124-143
468-473: LGTM: addToChannel prop provides good user control.The prop definition is clear, with a sensible default that automates the common case while allowing users to opt out when needed.
491-501: LGTM: Token selection logic is clear and correct.The methods properly implement the bot token precedence: use bot token when explicitly requested via
asBotand available, otherwise fall back to user token.
532-536: LGTM: SDK initialization properly supports token options.The method correctly passes options to
getTokenfor bot vs. user token selection and includesslackApiUrlconfiguration for custom Slack instances.
679-709: LGTM: Robust error handling for channel invitations.The method correctly:
- Checks for bot token availability
- Validates bot_id before proceeding
- Handles per-channel errors gracefully with logging
- Skips DM/group DM channels automatically (per the comment)
Note: Sequential channel processing (lines 694-705) could be slow for many channels, but this is likely necessary given Slack's API constraints.
948-973: LGTM: File methods correctly use bot token with clear documentation.The comments at lines 952-954 and 968-970 clearly explain that the bot token is required because the
files:readscope is only requested for bot tokens in the Pipedream app. This prevents auth errors when using file operations.components/slack/actions/find-message/find-message.mjs (2)
6-7: Verify the new docs endpoint and scope expectations.Confirm that assistant.search.context is the intended API reference for this action and that required scopes match the implementation and fallback logic.
163-170: Good fix: guarded includes on errorCode.The optional chaining prevents TypeError when errorCode is falsy. This resolves the earlier review concern.
| async realNameLookup(ids = [], usernames = [], throwRateLimitError = true, args = {}) { | ||
| let cursor; | ||
| const realNames = {}; | ||
| do { | ||
| const { | ||
| members: users, | ||
| response_metadata: { next_cursor: nextCursor }, | ||
| } = await this.usersList({ | ||
| limit: constants.LIMIT, | ||
| cursor, | ||
| throwRateLimitError, | ||
| ...args, | ||
| }); | ||
|
|
||
| for (const user of users) { | ||
| if (ids.includes(user.id)) { | ||
| realNames[user.id] = user.profile.real_name; | ||
| } | ||
| if (usernames.includes(user.name)) { | ||
| realNames[user.name] = user.profile.real_name; | ||
| } | ||
| } | ||
|
|
||
| cursor = nextCursor; | ||
| } while (cursor && Object.keys(realNames).length < (ids.length + usernames.length)); | ||
| return realNames; | ||
| }, |
There was a problem hiding this comment.
Fix potential infinite loop and performance issue.
Two issues in realNameLookup:
-
Infinite loop risk (line 676): If some IDs or usernames don't exist in the workspace,
realNameswill never reach the expected count, causing the loop to paginate indefinitely. -
O(n²) performance (lines 667, 670): Using
includes()on arrays inside a loop is inefficient for large lists.
Apply this diff to fix both issues:
- async realNameLookup(ids = [], usernames = [], throwRateLimitError = true, args = {}) {
+ async realNameLookup(ids = [], usernames = [], throwRateLimitError = true, args = {}) {
+ const idSet = new Set(ids);
+ const usernameSet = new Set(usernames);
let cursor;
const realNames = {};
+ const targetCount = ids.length + usernames.length;
do {
const {
members: users,
response_metadata: { next_cursor: nextCursor },
} = await this.usersList({
limit: constants.LIMIT,
cursor,
throwRateLimitError,
...args,
});
for (const user of users) {
- if (ids.includes(user.id)) {
+ if (idSet.has(user.id)) {
realNames[user.id] = user.profile.real_name;
}
- if (usernames.includes(user.name)) {
+ if (usernameSet.has(user.name)) {
realNames[user.name] = user.profile.real_name;
}
}
cursor = nextCursor;
- } while (cursor && Object.keys(realNames).length < (ids.length + usernames.length));
+ } while (cursor && Object.keys(realNames).length < targetCount);
return realNames;
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async realNameLookup(ids = [], usernames = [], throwRateLimitError = true, args = {}) { | |
| let cursor; | |
| const realNames = {}; | |
| do { | |
| const { | |
| members: users, | |
| response_metadata: { next_cursor: nextCursor }, | |
| } = await this.usersList({ | |
| limit: constants.LIMIT, | |
| cursor, | |
| throwRateLimitError, | |
| ...args, | |
| }); | |
| for (const user of users) { | |
| if (ids.includes(user.id)) { | |
| realNames[user.id] = user.profile.real_name; | |
| } | |
| if (usernames.includes(user.name)) { | |
| realNames[user.name] = user.profile.real_name; | |
| } | |
| } | |
| cursor = nextCursor; | |
| } while (cursor && Object.keys(realNames).length < (ids.length + usernames.length)); | |
| return realNames; | |
| }, | |
| async realNameLookup(ids = [], usernames = [], throwRateLimitError = true, args = {}) { | |
| const idSet = new Set(ids); | |
| const usernameSet = new Set(usernames); | |
| let cursor; | |
| const realNames = {}; | |
| const targetCount = ids.length + usernames.length; | |
| do { | |
| const { | |
| members: users, | |
| response_metadata: { next_cursor: nextCursor }, | |
| } = await this.usersList({ | |
| limit: constants.LIMIT, | |
| cursor, | |
| throwRateLimitError, | |
| ...args, | |
| }); | |
| for (const user of users) { | |
| if (idSet.has(user.id)) { | |
| realNames[user.id] = user.profile.real_name; | |
| } | |
| if (usernameSet.has(user.name)) { | |
| realNames[user.name] = user.profile.real_name; | |
| } | |
| } | |
| cursor = nextCursor; | |
| } while (cursor && Object.keys(realNames).length < targetCount); | |
| return realNames; | |
| }, |
🤖 Prompt for AI Agents
In components/slack/slack.app.mjs around lines 652 to 678, fix the potential
infinite loop and O(n²) performance by converting ids and usernames to Sets
before pagination, then on each page iterate members once and check membership
against those Sets (removing found entries from the Sets and mapping id/username
to real_name), and stop looping when either the Sets are empty (all requested
items resolved) or there is no next_cursor from usersList; also defensively
handle missing response_metadata/next_cursor to avoid relying on undefined and
thus prevent infinite pagination.
| assistantSearch(args = {}) { | ||
| args.count ||= constants.LIMIT; | ||
| return this.sdk().apiCall("assistant.search.context", { | ||
| ...args, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify whether assistantSearch should use makeRequest for consistency.
Unlike other methods, assistantSearch calls sdk().apiCall() directly, bypassing the retry logic and rate limit handling provided by makeRequest and _withRetries. This could lead to inconsistent behavior compared to other Slack API calls.
If the assistant.search.context API has different requirements that necessitate direct SDK usage, please document this with a comment. Otherwise, consider refactoring to use makeRequest for consistency.
🏁 Script executed:
#!/bin/bash
# Description: Check if other methods use direct apiCall instead of makeRequest
# Search for other instances of direct apiCall usage
rg -n "sdk\(\)\.apiCall" components/slack/Length of output: 96
🏁 Script executed:
#!/bin/bash
rg -n "this\\.makeRequest" components/slack/slack.app.mjs -C3Length of output: 7042
🏁 Script executed:
#!/bin/bash
# Search for makeRequest method definition
rg -n "makeRequest\s*\(" components/slack/slack.app.mjs -A10 | head -30Length of output: 1074
🏁 Script executed:
#!/bin/bash
# Search for _withRetries within makeRequest method
rg -n "_withRetries" components/slack/slack.app.mjs -C2 | head -40Length of output: 333
Refactor assistantSearch to use makeRequest for consistency with all other API methods.
The method uses direct sdk().apiCall() instead of makeRequest, bypassing the retry mechanism (3 retries) and rate-limit handling provided by _withRetries that every other API method in this file uses. This creates inconsistent error handling and resilience across the codebase.
At lines 895–900, change:
return this.sdk().apiCall("assistant.search.context", {
...args,
});To:
return this.makeRequest({
method: "assistant.search.context",
...args,
});🤖 Prompt for AI Agents
In components/slack/slack.app.mjs around lines 895 to 900, assistantSearch
currently calls this.sdk().apiCall directly (bypassing retries/rate-limit) and
should be changed to use this.makeRequest to get the same retry and rate-limit
behavior; set args.count default as before, then return this.makeRequest with
method: "assistant.search.context" and spread the rest of args so the call
signature and returned value remain identical but now uses the shared retry
wrapper.
1 participant
WHY
Summary by CodeRabbit
New Features
Improvements
Breaking Changes
Bug Fixes